Create HATAirdrop #542
Create HATAirdrop #542
Conversation
Create HATAirdrop
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
| bytes32 public root; | ||
| uint256 public startTime; | ||
| uint256 public deadline; | ||
| uint256 public lockEndTime; |
There was a problem hiding this comment.
we should consider lock length
There was a problem hiding this comment.
So here we can either just start from a certain date to a certain date, or we can start on claim and end after X time. I implemented the former but we the latter is also a valid option, we just need to decide what behavior we want here.
There was a problem hiding this comment.
-
the pattern "redeem -> tokenlock.release()
to get your tokens is pretty bad ux: it is hard to understand, and esp for small amounts it is a lot of gas costs. One limit case is where all tokens are already vested on theredeem()` call - in that case it would be much nicer (chepaer, easier) to just get the tokens directly (see my suggestion there). -
it would be cool to have some way to claim several airdrops in a single tx. Something like
factory.redeem(airDropContracts[], proofs[], amounts[], account)
contracts/tge/HATAirdropFactory.sol
Outdated
| implementation = _implementation; | ||
| } | ||
|
|
||
| function updateImplementation(address _newImplementation) external onlyOwner { |
There was a problem hiding this comment.
this is nice but it is not as flexible as it looks, bc any implementation contract needs to use the same signature for the initialization function. If we want the flexibility to create new airdrop contracts, we proably should pass the arguments encoded in a single parameter (but also that has it's drawbacks in terms of ux)
There was a problem hiding this comment.
Ok yeah it'd be more flexible
contracts/tge/HATAirdrop.sol
Outdated
| leafRedeemed[leaf] = true; | ||
|
|
||
| address _tokenLock = address(0); | ||
| if (lockEndTime != 0) { |
There was a problem hiding this comment.
| if (lockEndTime != 0) { | |
| if (lockEndTime > block.timestamp) { |
contracts/tge/HATAirdropFactory.sol
Outdated
| uint256 _lockEndTime, | ||
| uint256 _periods, | ||
| uint256 _totalAmount, | ||
| IERC20Upgradeable _token, |
There was a problem hiding this comment.
do we want to airdrop tokens different than hats? IF not, perhaps this argument should be in the constructor?
There was a problem hiding this comment.
I'm not sure if we might do different tokens or not
contracts/tge/HATAirdropFactory.sol
Outdated
| emit ImplementationUpdated(_newImplementation); | ||
| } | ||
|
|
||
| function withdrawTokens(IERC20Upgradeable _token, uint256 _amount) external onlyOwner { |
There was a problem hiding this comment.
So this allows for rugpulling the airdrop, as we discussed, and to mitigate we expect the funtion to be timelocked. Do we want a comment explaining that?
There was a problem hiding this comment.
If users do not redeem their tokens from the airdrop contract bf the deadline, hats can get those tokens back calling withdrawTokens. However, unclaimed tokens in the tokenlock will be lost forever. This can kind of be grief-attacked by an attacker that calls redeem on all unclaimed leafs just before the deadline (or even frontrunning withdrawTokens)
There was a problem hiding this comment.
So you'd recommend allowing only beneficiary to redeem for themselves right?
contracts/tge/HATAirdropFactory.sol
Outdated
| } | ||
|
|
||
| function withdrawTokens(IERC20Upgradeable _token, uint256 _amount) external onlyOwner { | ||
| address owner = owner(); |
There was a problem hiding this comment.
probably save a miniscule amount of gas
| address owner = owner(); | |
| address owner = msg.sender; |
This reverts commit d7d97b0.
Co-authored-by: Jelle <jellegerbrandy@gmail.com> Signed-off-by: benk10 <ben.kaufman10@gmail.com>
No description provided.